[TG-1058] Fixes parsing errors relating inner classes on generic classes #1512

thk123 · 2017-10-24T14:02:36Z

When parsing types that were inner classes inside generic classes

class Outer<T> {
  class Inner { }
  void foo(Inner input) {} 
  Inner field;
}

Then you end up with singatures like:

LOuter<TT;>Inner;

The parsing was only parsing up the the first ; or < meaning we missed out on the inner class.

This PR resolves this (and a linked issue of if the inner class is generic parsing the generic details from within it). Most of this PR is in the unit tests, so I suggest reviewing commit by commit. I will first squash the commits a bit. Then re-run clang-format to tidy them up.

thk123 · 2017-10-24T14:35:49Z

This is based on an old version of a develop (4955417) which is the current version on TG. This should be bumped before this is merged.

smowton

Looks tricky. Perhaps this is nasty enough that we need a real parser generator?

smowton · 2017-10-24T14:44:45Z

src/java_bytecode/java_types.cpp

+
+  // parse contained types, can be either type variables, starting with T
+  // or instantiated types
+  const std::vector<typet> &params =


Don't take by ref (it happens to work thanks to automagic lifetime extension, but it suggests something different to what's actually happening)

I though move semantics meant this was OK, but updated

smowton · 2017-10-24T14:45:44Z

src/java_bytecode/java_types.cpp

+
+  std::transform(params.begin(), params.end(),
+                 std::back_inserter(generic_type.generic_type_variables()),
+                 [](const typet &type) -> java_generic_parametert {


Kick this out into a non-member static function, it's too long for an inline lambda

My feeling is it is replacing the body of a for loop and therefore clearer to have it inline (particularly after applying below suggestions of returning directly). Can do if you disagree

It's actually a lot better now that its indentation is fixed

smowton · 2017-10-24T14:46:43Z

src/java_bytecode/java_types.cpp

+                 std::back_inserter(generic_type.generic_type_variables()),
+                 [](const typet &type) -> java_generic_parametert {
+                   if (is_java_generic_parameter(type)) {
+                     const java_generic_parametert &gen_type =


Just return it directly?

smowton · 2017-10-24T14:46:53Z

src/java_bytecode/java_types.cpp

+                   } else {
+                     INVARIANT(is_reference(type),
+                               "All generic parameters should be references");
+                     java_generic_inst_parametert inst_type(


Similarly, might as well return directly

smowton · 2017-10-24T14:51:33Z

src/java_bytecode/java_types.cpp

+/// \return The input string with everything between angle brackets removed
+std::string erase_type_arguments(const std::string &src) {
+  std::string class_name = src;
+  std::size_t f_pos = class_name.find('<', 1);


To avoid duplcating this at line 234, turn this into while(true) and simply put the break condition in the middle of the loop

You mean like:

std::size_t f_pos; while(true) { f_pos = class_name.find('<', 1); if(f_pos==std::string::npos) { break; } std::size_t e_pos = find_closing_delimiter(class_name, f_pos, '<', '>'); if(e_pos == std::string::npos) throw unsupported_java_class_signature_exceptiont("recursive generic"); // erase generic information between brackets class_name.erase(f_pos, e_pos - f_pos + 1); }

I think this makes the code more confusing to follow is it now not as clear what the loop is looping over.

smowton · 2017-10-24T15:06:03Z

src/java_bytecode/java_types.cpp

+    const typet &new_type = java_type_from_string(sub_str, class_name_prefix);
+    INVARIANT(new_type != nil_typet(), "Failed to parse type");
+
+    if (new_type.id() == ID_symbol) {


INVARIANT(new_type.id()!=ID_symbol)?

Sorry good spot - put in as erasing a condition I didn't think was relevant but forgot to change to an invariant.

In fact don't think it should be an invariant, but wanted to hard crash if anywhere that was true (it isn't particularly important here) so I've just removed it.

smowton · 2017-10-24T15:10:13Z

src/java_bytecode/java_types.cpp

+    size_t start = i;
+    while (i < src.size()) {
+      // parameter is an object type or instantiated generic type
+      if (src[i] == 'L') {


This looks very similar to java_type_from_string's function parameter list parsing, can they be factored?

Unless I've misunderstood this comment, it is exactly that code and this is it being factored so can be used for parsing <TT;Ljava/lang/Object;> type strings.

smowton · 2017-10-24T15:10:51Z

src/java_bytecode/java_types.cpp

+///   generic types
+/// \return The reference type if parsed correctly, no value if parsing fails.
+optionalt<reference_typet>
+build_class_name(const std::string src, const std::string &class_name_prefix) {


Take input string by ref?

smowton · 2017-10-24T15:12:31Z

src/java_bytecode/java_types.cpp

+  symbol_typet symbol_type(identifier);
+  symbol_type.set(ID_C_base_name, container_class);
+
+  std::size_t f_pos = src.find('<', 1);


Same loop rotation recommendation as above -- prefer writing this update condition once and doing while(true) { .... if(exit_condition) break; ... } over duplicating it like this

See above discussion, will apply both changes if you want, otherwise neither

smowton · 2017-10-24T15:26:03Z

src/java_bytecode/java_types.cpp

+            "There must be a semi-colon somewhere in the input");
+  size_t next_angle_bracket = src.find('<', starting_point);
+
+  while (next_angle_bracket < next_semi_colon) {


This might work, but I'd like to see some argument about why this exhaustively covers the type signature grammar... if this gets too tricky we should probably abandon manual parser writing and instead make a Bison or similar parse for the signature grammar (https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.3.4)

This exists but is GPL'd: https://github.com/JetBrains/jdk8u_jdk/blob/master/src/share/classes/sun/reflect/generics/parser/SignatureParser.java

Yes I completely agree, unfortunately there isn't time at this juncture to do this. Here is why I'm convinced this is reasonable for now:

This is for parsing a reference type from either a signature or descriptor. If it is descriptor, then the grammar is exactly:
L ClassName ;

So since ClassName can't contain ; or < we're done.

(Which is why it worked until now).

For signatures, it is more complicated:

L PackageSpecifier_opt SimpleClassTypeSignature ClassTypeSignatureSuffix* ;

The package specifier is just identifier/identifier so no chances of ; or <'

Then the SimpleClassTypeSignature is

Identifier TypeArguments_opt

Where type arguments must start with < (so this is the case where next_angle_bracket < next_semi_colon and must end with a corresponding >

This is then optionally followed by a . indicating inner class and then repeat above argument. It is finally ended by a semi colon.

I appreciate this is not convincing, hopefully the extensive unit tests are more convincing - though I accept they are a long way from an exhaustive proof.

mgudemann · 2017-10-24T15:13:46Z

src/java_bytecode/java_types.cpp

@@ -167,6 +167,243 @@ exprt java_bytecode_promotion(const exprt &expr)
    return typecast_exprt(expr, new_type);
 }

+std::vector<typet> parse_list_types(


shouldn't this be declared in the header file?

This is why I prefer classes even without state, as it allows declaring in the header without making a global function of everything that includes java_types.h I will move the declarations to the top of the file for now and raise a ticket to refactor this parsing into a class if this is satisfactory for you?

Done and raised as [TG-1206]

mgudemann · 2017-10-24T15:17:29Z

src/java_bytecode/java_types.cpp

+  const std::vector<typet> &params =
+    parse_list_types(parameters, class_name_prefix, '<', '>');
+
+  CHECK_RETURN(params.size() > 0); // We should have at least one generic param


use !..empty()

mgudemann · 2017-10-24T15:32:11Z

src/java_bytecode/java_types.cpp

+  {
+    std::size_t e_pos = find_closing_delimiter(class_name, f_pos, '<', '>');
+    if(e_pos == std::string::npos)
+      throw unsupported_java_class_signature_exceptiont("recursive generic");


I don't understand this, wouldn't this return npos if no corresponding closing > can be found, not if it is recursive generic?

You are right - however this case happens precisely when we have a recursive generic type. Given java_generic_type_from_string(<T:LSimpleRecursiveGeneric<TT;>;>Ljava/lang/Object;) calls erase type arguments with T:LSimpleRecursiveGeneric<TT (https://diffblue.atlassian.net/browse/TG-610)

Even after recursive generic types are fixed, the exception here makes sense. How about changing it to something more general like unsupported_java_class_signature_exceptiont("Failed to find generic signature closing delimiter"); or unsupported_java_class_signature_exceptiont("Failed to find generic signature closing delimiter (or recursive generic)"); if we want to keep the reminder that recursives cause problem here

mgudemann · 2017-10-24T15:34:40Z

src/java_bytecode/java_types.cpp

+  return class_name;
+}
+
+size_t find_closing_semi_colon_for_reference_type(


why not in the header?

As above - let me know if you'd be happy with my proposed solution

mgudemann · 2017-10-24T15:34:55Z

src/java_bytecode/java_types.cpp

+  const std::string src,
+  size_t starting_point = 0);
+
+/// Given a substring of a descriptor or signature that contains 1 or more types


mgudemann · 2017-10-24T15:37:48Z

src/java_bytecode/java_types.cpp

+    const typet &new_type = java_type_from_string(sub_str, class_name_prefix);
+    INVARIANT(new_type != nil_typet(), "Failed to parse type");
+
+    if(new_type.id() == ID_symbol)


why not an invariant with information string?

Removed as was only there as previously we did:

if(param.type().id()==ID_symbol) param.type()=java_reference_type(param.type());

but afaict this never happens (wanted to verify this with a hard fail - but since the CI passes and I can't see a way for this to happen, I guess it is just legacy code)

mgudemann · 2017-10-24T15:42:53Z

src/java_bytecode/java_types.cpp

+    {
+      std::size_t e_pos = find_closing_delimiter(src, f_pos, '<', '>');
+      if(e_pos == std::string::npos)
+        throw unsupported_java_class_signature_exceptiont("recursive generic");


We don't appear to have a test case that exercises this path so I will change the exception into a more prescriptive name

mgudemann · 2017-10-24T15:44:03Z

src/java_bytecode/java_types.cpp

+{
+  PRECONDITION(src[0] == 'L');
+  // ends on ;
+  if(src[src.size() - 1] != ';')


isn't this an invariant for any non-atomic class file level type ?

I thought so, changed this to an assertion and a test failed so I reverted back to original behaviour, but I can now no longer reproduce this, so perhaps I had something else wrong.

mgudemann · 2017-10-24T15:49:19Z

src/java_bytecode/java_types.cpp

+/// \return The string position corresponding to the matching ';'. For example:
+/// LA;, we would return 2. For LA<TT;>; we would return 7.
+/// See unit/java_bytecode/java_util_tests.cpp for more examples.
+size_t find_closing_semi_colon_for_reference_type(


would it make sense to adapt the find_closing_delimiter function to take a set of opening characters? Then using find_closing_delimiters(src, end, "LT", ;) would return the ; that corresponds to the first L and skip over any other L,; or T, ; pair

I think the problem is you have two types of brackets L, T, which are closed by ; and < which is closed by >. I suppose it would in fact work as Lfoo<;>; isn't legal, but presumably wouldn't bec caught if that did happen. Also LL; would fail (class named L)

And indeed the far more common Lgeneric_class<TT;>;

the idea of find_closing_delimiters is to support hierarchic structures (maybe it's buggy?) and allow for free definition of closing and opening delimiter.
What I imagined was to adapt it in such a way that it would in this case

ignore < and >

increase the depth at T and L

decrease depth at ;

agreed for LL; or LT;, but with one lookahead or a Boolean that indicates that the last char was an L and "opened" the object name, we would be good, wouldn't we?
This parsing is slightly fragile and with one common function, we'd at least know where to correct an error and don't risk overseeing another place.

Surely if you ignore <, there is no difference between LALFoo; (a class called ALFoo) and LA<LFoo;>; and in the first case you won't find a matching ;?

I'll give it a go but I think this is a fudamentally different task (matching two isolated sets of brackets) and I think it's a bad idea to complicate find_closing_delimiters

keep it like that then, with the overall goal to get in a real parser, I am ok with this

mgudemann · 2017-10-24T16:00:24Z

unit/java_bytecode/java_utils_test.cpp

@@ -13,6 +13,7 @@

 #include <testing-utils/catch.hpp>

+#include <java_bytecode/java_types.cpp>


including the cpp file here? is this the reason for the definitions in the cpp file instead of the header ?

Vice-versa, including as declarations are not in the header, not in the header to avoid making the functions global. See earlier discussion and let me know if you're happy with my proposed solution (this will still include the cpp file)

thk123 · 2017-10-24T17:15:53Z

Thanks for the reviews - I've updated PR (didn't squash commits to make it easier to review changes - but indicated in commit message what they would be squashed into).

Will address the liner issues tomorrow but let me know about the comments I've commented on @smowton @mgudemann

mgudemann · 2017-10-25T08:33:12Z

@thk123 won't fight for the parsing stuff, but we should maybe think of using a parser generator and implement the grammar given in the spec.

thk123 · 2017-10-25T09:29:28Z

@mgudemann @smowton I have raised a ticket TG-1205 to convert this conversion to using a parser generator since this logic is clearly confusing and error prone.

I will try using find_closing_delimiter but other than that I think this is ready for re-review

majakusber

Looks good, I only had three small things - one of them you already fixed (I was looking at it commit by commit).

majakusber · 2017-10-25T09:14:56Z

src/java_bytecode/java_types.cpp

+  {
+    std::size_t e_pos = find_closing_delimiter(class_name, f_pos, '<', '>');
+    if(e_pos == std::string::npos)
+      throw unsupported_java_class_signature_exceptiont("recursive generic");


Even after recursive generic types are fixed, the exception here makes sense. How about changing it to something more general like unsupported_java_class_signature_exceptiont("Failed to find generic signature closing delimiter"); or unsupported_java_class_signature_exceptiont("Failed to find generic signature closing delimiter (or recursive generic)"); if we want to keep the reminder that recursives cause problem here

majakusber · 2017-10-25T09:20:20Z

unit/java_bytecode/java_utils_test.cpp

+  }
+  GIVEN("Signature: An generic inner class in a generic class")
+  {
+    std::string signature = "LClassName<TT;>.Inner<VV;>;";


Should it be TV instead of VV here? (similarly in multiple places in this scenario) I thought all type variables are given prefix T.

majakusber · 2017-10-25T09:28:10Z

src/java_bytecode/java_types.cpp

+  {
+    std::size_t e_pos = find_closing_delimiter(src, f_pos, '<', '>');
+    if(e_pos == std::string::npos)
+      throw unsupported_java_class_signature_exceptiont("recursive generic");


Same as in the other file, the exception makes sense in general, not only for recursive generics, consider changing to something like unsupported_java_class_signature_exceptiont("Failed to find generic signature closing delimiter");

thk123 · 2017-10-25T12:25:33Z

Remaining linter errors are discrepancies between clang-format and cpplint.py - there is an on-going discussion as to what to do with these

smowton

LGTM

When parsing fields that are of the inner class type, but the outer class is generic, we wrongly parse this a pointer to the outer type. This test demonstrates this problem.

Skip over the signature when building the class name.

Previously when a generic signature was parsed, inner classes were ignored. This resolves this by first building up a complete class name. This also resolves the fact that inner classes in signatures are denoted by . rather than $ and so replaces the . with $ to be consistent.

This is consistent with the original behaviour which would return a nil_typet

… param It is not possible for a generic parameter to either have multiple names or have multiple instantiated types so the public interface should only return one value.

When there is a function on a generic type whose params are inner classes we would wrongly parse only up to the inner class dot for the name. This resulted in wrong parameters causing us to fall back on the descriptor unnecessarily. Instead we should find the semi-colon that matches the L for each parameter before moving on to the next parameter.

Use the logic to parse multiple parameters for generic type parameters.

…ectly.

We don't have a test that exercises this exception so modified the message to not mislead and say to do with recursive generic types.

Assumed that the bracket was always an angle bracket

thk123 · 2017-10-26T09:25:17Z

Rebased - just waiting on CI then good to merge

4820601 Merge remote-tracking branch 'diffblue/develop' into merge-dev-to-ss 278f309 Merge pull request diffblue#1526 from reuk/reuk/fixup-unified-diff 69ba9e7 Merge pull request diffblue#1546 from smowton/smowton/feature/expose_tarjan 05dc65c Use lower-case characters to start error messages 8d0b23b Change asserts to invariants a7afc6c Return from get_diff by value 6833af6 Fix formatting 4555d9f Remove unused parameter e32c6c5 Make instructions_equal static f815cc0 Move instructions_equal definition 11c9acc Add getter for data member 8c35211 Make data member private f54db47 Rename differences map data member b67d6e8 Merge pull request diffblue#1543 from chrisr-diffblue/cleanup/add-java-array-element-type-helper c9241cd Make generic Tarjan's algorithm publically available ebabdb9 Merge pull request diffblue#1542 from smowton/smowton/cleanup/sharing-node-assertion a523d6f Add a helper function for accessing the element type of a Java array type d8e8a68 Use INVARIANT rather than assert for sharing-node assertions 64d5dd2 Merge pull request diffblue#1538 from smowton/smowton/cleanup/remove-returns-asserts-to-invariants fca7c04 Merge pull request diffblue#1537 from smowton/smowton/cleanup/sharing-node-test-to-catch e35372a Merge pull request diffblue#1539 from romainbrenguier/bugfix/char-array-in-java-strings#TG-58#newpr 4d011b5 Merge pull request diffblue#1541 from owen-jones-diffblue/replace-unsigned-with-number-type b2a4e39 Merge pull request diffblue#1540 from owen-jones-diffblue/rename-detach 2f6ceed Replace unsigned with appropriate type 938ab2b Replace 'detatch' with 'detach' c219663 Convert sharing-node test to run under Catch dec5ddf Remove-returns: asserts -> invariants 3b1f485 Merge pull request diffblue#1535 from andreast271/ignore-eclipse-vs-dirs 6cea5aa Ignore files and directories created by Eclipse and Visual Studio aa94fe8 Style: add nolint marker on lines formated by clang ee4a887 Merge pull request diffblue#1458 from LAJW/perl-platform 4da5a94 Add templating functionality to the perl script 21b2641 Move make_function_application to java_utils 5196e40 Merge pull request diffblue#884 from nmanthey/ipasir-no-ci 3ee3b25 Refactor: remove m_ prefix from member fields 539ff9f Refactoring: simplify and remove unused expression 5a0d6b4 Rename first_index in array_expr d03b8cc Remove regression test that is not checking anything 90c8495 Improve tests for StringBuilder.append([C) 232617c Add code for String constructor from array a2d7811 Use dynamic object instead of tmp_object in init daef30f Splitting is_string and init_string parts of init ad65847 Style: rename i in chr_int to avoid clash e59349f Remove redundant check in from_int_with_radix 038b476 Allow index for argument of associate array to pointer 73d51fc Remove insert_long which duplicates insert_int bb22700 Cleanup unused fields of constraint generator 2355e8e Fix set of created strings in generator and use it 26e895d Distinguish strings and char arrays in get 4b8a421 Make get_array return array of unknown expr ca8213f Simplify not_contains constraints before negation 92897f7 Style fixes in string_constraint_generator.h 145364c String refinement: Improve debug information 3e5b3f1 Minor code improvements in string refinement 3d5465e Minor code improvements in generator_insert 86e1444 Refactoring: use begin()+3 instead of 3 times next fef1c5f Correction in debug model 28590fe Correction in constraints for concat 6861b9d Documentation fixes in stirng constraint generator c410159 Regression: StringBuilder append with int argument e6700ff Minor improvements in bytecode typecheck 61f0e1b Making check_axioms for string_constraints and not_contains_constraints more uniform f2122c6 Correct signature of convert_exprt_to_string_exprt_unit_test 9edbb90 [string-refinement] Change get_array to return optional 86e4782 [string-refinement] Allow index set saturation if not_contains_constraints are present c9c612d [string-refinement] Do not update index set of constant arrays b88fe35 [string-refinement] Check for char type 1a22916 [string-refinement] Display debug info for index-set 982a5fc Style and documentation fixes in preprocessing a83daa7 Minor indentation, naming and const-fixes 15fd1b4 Fix typos in strings and comments of string solver 26ae9a9 Unit test improve convert exprt to string exprt b76b116 Minor improvements in string preprocessing 05a6b09 [constraint-generator] Removing declarations of unimplemented functions 302f92e Make string primitives return return_code_type b80d063 Doxygen corrections 369dd62 Unit test: adapte instantiate_nc for new signature dcae158 [string-refinement] Removing unused functions b6b2669 Style changes in string_constraint_generator_testing edf7057 Complete string solver rework 9ea2eda Unit tests for union_find_replacet 4adf5d0 New class for union find replace 1d112d5 Java object factory for nondet strings 0ae71b0 [string-preprocessing] New functions for calling string primitives in initialization e89f5e4 [string-preprocess] Refactor java_types_matches_tag 2505982 Extra preconditions in string solver 6378400 Style improvements in the string solver 36d6e6d [util/irep_ids] Additional identifiers fa45b34 Regression test mprovement for if expressions 76cd14d Unit test for gen_nondet_string_init c71c64b Regression: include model for string to char array c3d527c compiling: add IPASIR notes for Linux 900a0fc tests: do not match iterations line 751208d tests: drop number of iterations 12a6917 build: add ipasir solver support 14419d2 solvers: add ipasir driver 8aa89be build: introduce LIBSOLVER environment variable df45bdb Merge pull request diffblue#1524 from reuk/reuk/fix-linter 4b39446 Merge pull request diffblue#1518 from diffblue/taint-for-C 25e18c0 Merge pull request diffblue#1529 from smowton/smowton/fix/float128 3cf67f7 Merge pull request diffblue#1531 from reuk/reuk/expr-cast-fixup 2169a39 Add _Float128 spelling for 128-bit float type a3cf849 Merge pull request diffblue#1530 from martin-cs/feature/document-return-codes 188f263 Enable casting from derived types to other derived types 3dd7f6c first regression tests 29bc3b8 custom_bitvector_domain: allow objects that are members ab7270e check taint on sinks _before_ the call 6daa8bd Merge pull request diffblue#1528 from tautschnig/shl-overflow 2df2abb Change a few erronious return codes so that they are more internally consistent. cf96cef Minor changes to erronious exit behaviour. c8dfd48 Replace constants exit codes with meaningfully named macros. 961d33a Replace literal constants in returns / exits with their symbolic names. e74b442 Named, possibly even descriptive macros for the exit codes that are used. 1e3712c Shifts of non-integers and left shifts of negative integers are undefined 6bb3872 Check for overflow on left shift of signed ints f9af374 Merge pull request diffblue#1512 from thk123/bugfix/TG-1058/crash-inner-class-generic-class bd2e9c2 Merge pull request diffblue#1522 from reuk/reuk/clang-format-plain-diff f892f4a Added tests for bracket matcher to include different types of brackets 3beab8b Fixed error in find_closing_delimiter 0b3058f Tightened up exception for unknown handling 9f87a80 Assertion on unmatched ; for parsing reference types 5922826 Adding doubly nested generic tests 4a62b07 Simplifying method names for unit test ee39620 Extended tests to verify the generic information is being parsed correctly. 465a473 Parse multiple nested generic type informations b8d43ab Pulled out method for parsing a list of multiple types 2a5c280 Correctly parse multiple input parameters for functions 35d974f Add method for finding the matching ; corresponding to a reference L e1621a3 Hide methods relating to getting multiple type variables for specific param e952708 Adding tests for parsing methods of generic classes with inner classes 958c006 Adding utilities for code_type structures 72a041b Don't crash when found an invalid reference type ed545cb Refactored out the code for erasing generic types effc1b2 Parse the whole class when generics are present d23a0cc Added function for getting the full class name of a type 34c185e Unit test reproducing the bug described in TG-1058 5b92002 Adding utilities for checking types in unit tests a00edd3 Disable dowhile brace check by default d38b5d8 Disable colour in travis diff display git-subtree-dir: cbmc git-subtree-split: 4820601

thk123 requested review from mgudemann and smowton as code owners October 24, 2017 14:02

thk123 changed the title ~~[TG-1058]~~ [TG-1058] Fixes parsing errors relating inner classes on generic classes Oct 24, 2017

thk123 force-pushed the bugfix/TG-1058/crash-inner-class-generic-class branch from 26136ab to af823b1 Compare October 24, 2017 14:34

thk123 added the do not merge label Oct 24, 2017

thk123 force-pushed the bugfix/TG-1058/crash-inner-class-generic-class branch from af823b1 to ff09333 Compare October 24, 2017 14:40

thk123 requested a review from majakusber October 24, 2017 14:51

thk123 force-pushed the bugfix/TG-1058/crash-inner-class-generic-class branch from ff09333 to dc6fb60 Compare October 24, 2017 14:54

smowton reviewed Oct 24, 2017

View reviewed changes

mgudemann suggested changes Oct 24, 2017

View reviewed changes

mgudemann approved these changes Oct 25, 2017

View reviewed changes

majakusber approved these changes Oct 25, 2017

View reviewed changes

smowton approved these changes Oct 25, 2017

View reviewed changes

thk123 force-pushed the bugfix/TG-1058/crash-inner-class-generic-class branch 2 times, most recently from 54700d8 to 0ce5035 Compare October 26, 2017 09:22

thk123 added 10 commits October 26, 2017 10:24

Adding utilities for checking types in unit tests

5b92002

Unit test reproducing the bug described in TG-1058

34c185e

When parsing fields that are of the inner class type, but the outer class is generic, we wrongly parse this a pointer to the outer type. This test demonstrates this problem.

Added function for getting the full class name of a type

d23a0cc

Skip over the signature when building the class name.

Refactored out the code for erasing generic types

ed545cb

Don't crash when found an invalid reference type

72a041b

This is consistent with the original behaviour which would return a nil_typet

Adding utilities for code_type structures

958c006

Adding tests for parsing methods of generic classes with inner classes

e952708

Hide methods relating to getting multiple type variables for specific…

e1621a3

… param It is not possible for a generic parameter to either have multiple names or have multiple instantiated types so the public interface should only return one value.

Add method for finding the matching ; corresponding to a reference L

35d974f

thk123 added 10 commits October 26, 2017 10:24

Pulled out method for parsing a list of multiple types

b8d43ab

Parse multiple nested generic type informations

465a473

Use the logic to parse multiple parameters for generic type parameters.

Extended tests to verify the generic information is being parsed corr…

ee39620

…ectly.

Simplifying method names for unit test

4a62b07

Adding doubly nested generic tests

5922826

Assertion on unmatched ; for parsing reference types

9f87a80

Tightened up exception for unknown handling

0b3058f

We don't have a test that exercises this exception so modified the message to not mislead and say to do with recursive generic types.

Fixed error in find_closing_delimiter

3beab8b

Assumed that the bracket was always an angle bracket

Added tests for bracket matcher to include different types of brackets

f892f4a

thk123 force-pushed the bugfix/TG-1058/crash-inner-class-generic-class branch from 0ce5035 to a90a164 Compare October 26, 2017 09:24

thk123 removed the do not merge label Oct 26, 2017

thk123 force-pushed the bugfix/TG-1058/crash-inner-class-generic-class branch from a90a164 to f892f4a Compare October 26, 2017 10:12

thk123 merged commit f9af374 into diffblue:develop Oct 26, 2017

thk123 deleted the bugfix/TG-1058/crash-inner-class-generic-class branch October 26, 2017 13:02

		@@ -13,6 +13,7 @@

		#include <testing-utils/catch.hpp>

		#include <java_bytecode/java_types.cpp>

[TG-1058] Fixes parsing errors relating inner classes on generic classes #1512

[TG-1058] Fixes parsing errors relating inner classes on generic classes #1512

Uh oh!

Conversation

thk123 commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thk123 commented Oct 24, 2017

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thk123 Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thk123 commented Oct 24, 2017 •

edited

Loading

thk123 Oct 24, 2017 •

edited

Loading